Finish Ethereum and Dummy adapter by samparsky · Pull Request #143 · AmbireTech/adex-validator-stack-rust

samparsky · 2019-09-19T05:48:15Z

Ivshti · 2019-09-25T08:30:58Z

adapter/contract/AdExCore.json

@@ -0,0 +1 @@
+[{"constant":true,"inputs":[{"name":"","type":"bytes32"}],"name":"withdrawn","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"name":"","type":"bytes32"},{"name":"","type":"address"}],"name":"withdrawnPerUser","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"components":[{"name":"creator","type":"address"},{"name":"tokenAddr","type":"address"},{"name":"tokenAmount","type":"uint256"},{"name":"validUntil","type":"uint256"},{"name":"validators","type":"address[]"},{"name":"spec","type":"bytes32"}],"name":"channel","type":"tuple"}],"name":"channelOpen","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"components":[{"name":"creator","type":"address"},{"name":"tokenAddr","type":"address"},{"name":"tokenAmount","type":"uint256"},{"name":"validUntil","type":"uint256"},{"name":"validators","type":"address[]"},{"name":"spec","type":"bytes32"}],"name":"channel","type":"tuple"},{"name":"stateRoot","type":"bytes32"},{"name":"signatures","type":"bytes32[3][]"},{"name":"proof","type":"bytes32[]"},{"name":"amountInTree","type":"uint256"}],"name":"channelWithdraw","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"components":[{"name":"creator","type":"address"},{"name":"tokenAddr","type":"address"},{"name":"tokenAmount","type":"uint256"},{"name":"validUntil","type":"uint256"},{"name":"validators","type":"address[]"},{"name":"spec","type":"bytes32"}],"name":"channel","type":"tuple"}],"name":"channelWithdrawExpired","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"name":"","type":"bytes32"}],"name":"states","outputs":[{"name":"","type":"uint8"}],"payable":false,"stateMutability":"view","type":"function"},{"anonymous":false,"inputs":[{"indexed":true,"name":"channelId","type":"bytes32"}],"name":"LogChannelOpen","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"channelId","type":"bytes32"},{"indexed":false,"name":"amount","type":"uint256"}],"name":"LogChannelWithdrawExpired","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"channelId","type":"bytes32"},{"indexed":false,"name":"amount","type":"uint256"}],"name":"LogChannelWithdraw","type":"event"}]


can't you include adex-protocol-eth as a submodule?

I have included it

Ivshti · 2019-09-25T08:31:07Z

adapter/src/dummy.rs

@@ -1,7 +1,7 @@
 #![deny(clippy::all)]
 #![deny(rust_2018_idioms)]


why is this needed?

rust-lang/rust#52047
Some good lints for the 2018 edition

Ivshti · 2019-09-25T08:33:08Z

adapter/src/dummy.rs

-        // @TODO
-        Ok("hello".to_string())
+    fn session_from_token(&mut self, token: &str) -> AdapterResult<Session> {
+        let mut identity = "";


you should use a functional style here
let identity = self.tokens_for_auth.iter().find(|(key, val)| val == token)

Ivshti · 2019-09-25T08:33:54Z

adapter/src/dummy.rs

+
+        match who {
+            Some((id, _)) => {
+                let auth = self.tokens_for_auth.get(&id).unwrap();


this unwrap should be expect so that you can explain why you make that assumption

Ivshti · 2019-09-25T08:34:42Z

adapter/src/ethereum.rs

+            None,
+            &Some(self.keystore_pwd.clone()),
+        )
+        .expect("Failed to create account");


why doesn't this just return an Err?

Ivshti · 2019-09-25T08:37:13Z

adapter/src/ethereum.rs

+                    public_to_address(
+                        &wallet
+                            .public(&self.keystore_pwd)
+                            .expect("failed to get public key")


same...err handling should use Err

Ivshti · 2019-09-25T08:38:08Z

adapter/src/ethereum.rs

+                    contract_address,
+                    include_bytes!("../contract/Identity.json"),
+                )
+                .unwrap();


this is OK to be unwrapped, but you should use lazy_static for this

Ivshti · 2019-09-25T08:38:35Z

adapter/src/ethereum.rs

+
+                let contract_query =
+                    contract.query("privileges", verified.from, None, Options::default(), None);
+                let priviledge_level: U256 = contract_query.wait().unwrap();


let's keep this like that for now
but we should change this to an async call ASAP

Ivshti · 2019-09-25T08:39:19Z

adapter/src/ethereum.rs

+            (None, Some(wallet)) => {
+                let payload = Payload {
+                    id: validator.id.clone(),
+                    era: usize::try_from(Utc::now().timestamp()).expect("failed to parse utc now"),


this is an actual OK case for expect! cause there's no realistic way it can fail

Ivshti · 2019-09-25T08:39:58Z

adapter/src/ethereum.rs

+        );
+
+        // Sign
+        let expected_response =


those match with the JS impl, right?

yes it does

adapter/src/dummy.rs

elpiel · 2019-09-30T13:14:53Z

adapter/src/lib.rs

    pub token_addr: String,
    pub token_amount: String,
-    pub valid_until: String,
+    pub valid_until: i64,


is this milliseconds (can we document this)? Should we try to use DateTime?

i64 works fine but the param to new requires Datetime

Yeah, I mean it's more about documenting it, creating the EthereumChannel requires Datetime, but when you use the public field valid_until from an object, you don't know where it comes from and what it is.

adapter/src/lib.rs

adapter/src/ethereum.rs

primitives/src/util/tests/prep_db.rs

validator_worker/src/lib.rs

samparsky · 2019-10-01T10:13:37Z

@elpiel The suggested changes have been made

adapter/src/ethereum.rs

elpiel · 2019-10-04T11:26:23Z

validator_worker/src/sentry_interface.rs

+        let whoami = adapter.whoami();
+
        let validator = self
            .channel
            .spec
            .validators
            .into_iter()
            .find(|&v| v.id == whoami);
-        let auth_token = self
-            .adapter
-            .get_auth(validator.unwrap())
-            .expect("Failed to get user auth token");
+
+        let auth_token = adapter.get_auth(validator.unwrap());


Even though we have fairly small amount of code, I see the confusion that leads to duplicating code by accident.

Here is what I don't understand: Why do we even bother to do this check, if we already have it in the initialization of the SentryInterface?
Why search for the adapter.whoami() in the channle.spec.validators and then even validator.unwrap() the value?
This whole thing can be simplified to just:

let auth_token = adapter.get_auth(adapter.whoami());

If you follow the implementation, the adapter.whoami() will always be included in the Channel validators, but it's just not obvious.

adapter.whoami() returns a string,
.find(|&v| v.id == whoami); returns a ValidatorDesc

Also, the idea behind the unwrap() is because we already check it in new() so it would always be there

elpiel · 2019-10-04T11:29:59Z

validator_worker/src/sentry_interface.rs

-            .expect("Failed to get user auth token");
+
+        let auth_token = adapter.get_auth(validator.unwrap());
+        if let Err(e) = auth_token {


You could map this error and use ? and get rid off the unwrap() bellow.

Same way you did here: https://github.com/AdExNetwork/adex-validator-stack-rust/pull/143/files/e4739e44f81d4c040f627043aa33bbb160c21a71..a781eb41a68ffa5cd31c3b42bfb2de638f773d29#diff-ae6ae3e6a4852afdf6f5eda167faccd7R189

samparsky force-pushed the ethereum-adapter branch from 2fe1017 to cd96bae Compare September 20, 2019 14:26

samparsky marked this pull request as ready for review September 20, 2019 14:26

samparsky force-pushed the ethereum-adapter branch from 66f7e89 to f5f806a Compare September 21, 2019 11:23

samparsky added 14 commits September 24, 2019 12:15

fix: ethereum sign

65f1645

implement: ethereum adapter

ef99520

fix: decoded base64 lifetime

5f0e494

fix: ethereum adapter

3f3fa3b

fix: ethereum signed messages

7f1b642

fix: ethereum adapter sign, verify, ewt_sign, ewt_verify

102e983

fix: ethereum adapter address checksum

4a734a3

fix: adapter formatting

29d95c9

fix: format adapter/lib.rs, remove unused code

31c9b94

fix: validator_worker/heartbeat.rs, sentry_interface.rs mutable adapter

3f29db9

update: ethsign crate default

208f9c7

fix: remove ethsign crate

1e4cb4f

fix: box adapter types

7e9a289

changes

bd5404e

Ivshti requested changes Sep 25, 2019

View reviewed changes

fix: ethereum adapter, remove mut

6c1a330

samparsky force-pushed the ethereum-adapter branch from f5f806a to 6c1a330 Compare September 26, 2019 21:08

fix: add generic adapter error

6a824f3

samparsky force-pushed the ethereum-adapter branch from 8fcc1b2 to 6a824f3 Compare September 27, 2019 10:34

add: protocol eth submodule

ffec5e2

samparsky force-pushed the ethereum-adapter branch 3 times, most recently from 7aaa0c5 to 11b1355 Compare September 27, 2019 11:55

fix: keystore json err message

cb14290

samparsky force-pushed the ethereum-adapter branch from 11b1355 to cb14290 Compare September 27, 2019 12:03

fix: check_address_checksum eth adapter

a28fa02

samparsky force-pushed the ethereum-adapter branch from 92d1536 to 5337baf Compare September 29, 2019 19:26

fix: ethereum adapter whoami

8fb4304

samparsky force-pushed the ethereum-adapter branch from 2c09fce to 8fb4304 Compare September 30, 2019 10:25

elpiel suggested changes Sep 30, 2019

View reviewed changes

samparsky force-pushed the ethereum-adapter branch from ad941aa to d86a7e8 Compare September 30, 2019 16:04

fix: ethereum, dummy adapter

e4739e4

samparsky force-pushed the ethereum-adapter branch from d86a7e8 to e4739e4 Compare September 30, 2019 16:05

elpiel reviewed Oct 1, 2019

View reviewed changes

adapter/src/ethereum.rs Show resolved Hide resolved

elpiel previously approved these changes Oct 1, 2019

View reviewed changes

elpiel requested a review from Ivshti October 1, 2019 11:40

Ivshti previously approved these changes Oct 3, 2019

View reviewed changes

samparsky dismissed stale reviews from Ivshti and elpiel via a781eb4 October 4, 2019 10:02

elpiel suggested changes Oct 4, 2019

View reviewed changes

samparsky force-pushed the ethereum-adapter branch from a781eb4 to 4e50236 Compare October 4, 2019 14:30

samparsky requested review from Ivshti and elpiel October 4, 2019 15:12

samparsky force-pushed the ethereum-adapter branch 2 times, most recently from 0a58df2 to cf785d4 Compare October 4, 2019 15:30

fix: remove RefCell adapter, implement RwLock sentry_interface

8be6f50

samparsky force-pushed the ethereum-adapter branch from cf785d4 to 8be6f50 Compare October 4, 2019 15:31

elpiel previously approved these changes Oct 7, 2019

View reviewed changes

elpiel mentioned this pull request Oct 9, 2019

Split AdapterOptions #152

Closed

fix: make adapter options enum and tryfrom channel to ethchannel

6d55e48

samparsky dismissed elpiel’s stale review via 6d55e48 October 9, 2019 12:48

samparsky requested a review from elpiel October 9, 2019 12:56

elpiel approved these changes Oct 9, 2019

View reviewed changes

samparsky merged commit 6652af3 into AmbireTech:dev Oct 9, 2019

This was referenced Oct 24, 2019

Refactor: Implement Ethereum Adapter #121

Closed

Dummy adapter #53

Closed

		@@ -0,0 +1 @@
		[{"constant":true,"inputs":[{"name":"","type":"bytes32"}],"name":"withdrawn","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"name":"","type":"bytes32"},{"name":"","type":"address"}],"name":"withdrawnPerUser","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"components":[{"name":"creator","type":"address"},{"name":"tokenAddr","type":"address"},{"name":"tokenAmount","type":"uint256"},{"name":"validUntil","type":"uint256"},{"name":"validators","type":"address[]"},{"name":"spec","type":"bytes32"}],"name":"channel","type":"tuple"}],"name":"channelOpen","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"components":[{"name":"creator","type":"address"},{"name":"tokenAddr","type":"address"},{"name":"tokenAmount","type":"uint256"},{"name":"validUntil","type":"uint256"},{"name":"validators","type":"address[]"},{"name":"spec","type":"bytes32"}],"name":"channel","type":"tuple"},{"name":"stateRoot","type":"bytes32"},{"name":"signatures","type":"bytes32[3][]"},{"name":"proof","type":"bytes32[]"},{"name":"amountInTree","type":"uint256"}],"name":"channelWithdraw","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"components":[{"name":"creator","type":"address"},{"name":"tokenAddr","type":"address"},{"name":"tokenAmount","type":"uint256"},{"name":"validUntil","type":"uint256"},{"name":"validators","type":"address[]"},{"name":"spec","type":"bytes32"}],"name":"channel","type":"tuple"}],"name":"channelWithdrawExpired","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"name":"","type":"bytes32"}],"name":"states","outputs":[{"name":"","type":"uint8"}],"payable":false,"stateMutability":"view","type":"function"},{"anonymous":false,"inputs":[{"indexed":true,"name":"channelId","type":"bytes32"}],"name":"LogChannelOpen","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"channelId","type":"bytes32"},{"indexed":false,"name":"amount","type":"uint256"}],"name":"LogChannelWithdrawExpired","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"channelId","type":"bytes32"},{"indexed":false,"name":"amount","type":"uint256"}],"name":"LogChannelWithdraw","type":"event"}] No newline at end of file

		@@ -1,7 +1,7 @@
		#![deny(clippy::all)]
		#![deny(rust_2018_idioms)]

Conversation

samparsky commented Sep 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elpiel Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samparsky Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samparsky commented Oct 1, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elpiel Sep 25, 2019 •

edited

Loading

samparsky Oct 1, 2019 •

edited

Loading